Remove generated InstrumentationModule#getMuzzleReferences() method from the source code #4087
Conversation
…rom the source code
muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/MuzzleReferencesAccessor.java
Outdated
Show resolved
Hide resolved
| @@ -36,7 +37,14 @@ | |||
| private final Set<String> helperClassNames; | |||
| private final InstrumentationClassPredicate instrumentationClassPredicate; | |||
|
|
|||
| public ReferenceMatcher( | |||
| public static ReferenceMatcher of(InstrumentationModule instrumentationModule) { | |||
iNikem
requested review from
anuraaga,
jkwatson,
laurit,
pavolloffay and
tylerbenson
as code owners
muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/MuzzleReferencesAccessor.java
Outdated
Show resolved
Hide resolved
| */ | ||
| class MuzzleReferencesAccessor { | ||
| private static final ClassValue<MethodHandle> getMuzzleReferences = | ||
| new ClassValue<MethodHandle>() { |
There was a problem hiding this comment.
Now that I think of it we don't really need to use ClassValues and MethodHandles - the getMuzzleReferences() will be called exactly once per InstrumentationModule implementation, so there's no need to cache the result here (it's already cached in the MuzzleMatcher).
There was a problem hiding this comment.
Are you sure? Will be happy to remove this cache!
There was a problem hiding this comment.
Yep -- the only place (MuzzleMatcher) that constructs ReferenceMatcher in the agent uses an AtomicBoolean initialized field to make sure it only does it once (building a giant map of references can get expensive).
There was a problem hiding this comment.
What about io.opentelemetry.javaagent.tooling.muzzle.ClassLoaderMatcher#matchesAll?
There was a problem hiding this comment.
Also, I will try Lauri idea from #4087 (comment) anyway later on. That will probably remove this whole class.
There was a problem hiding this comment.
That one is only used in the gradle muzzle-check plugin, it doesn't affect the agent at all.
| * generated automatically during compilation by the {@code | ||
| * io.opentelemetry.instrumentation.javaagent-codegen} Gradle plugin. | ||
| */ | ||
| class MuzzleReferencesAccessor { |
There was a problem hiding this comment.
Perhaps javaagent-codegen should add an interface that has getMuzzleReferences to instrumentation module so you wouldn't need to use reflection.
There was a problem hiding this comment.
I assume that MethodHandle has no performance overhead on invocation. But I will think about this idea, it may be more elegant indeed.
There was a problem hiding this comment.
I will try this idea as a follow-up optimisation. I like it, but prefer to finish current iteration of removing ClassRef and friends from public API.
There was a problem hiding this comment.
This will also be easier to extend with other muzzle-generated methods.
There was a problem hiding this comment.
If it is possible to build extension jars that contain instrumentation modules then all of the muzzle and codegen stuff has become part of our api. Have you thought about how to version it and how to handle cases where agent is incompatible with codegen version used?
There was a problem hiding this comment.
Ohhh, good question... The only vague thought I have atm is "javaagent must support all previous stable versions of codegen within any given major version." Which assumes "in sync" versioning of the agent and gradle plugins.
There was a problem hiding this comment.
If it is possible to build extension jars that contain instrumentation modules then all of the muzzle and codegen stuff has become part of our api
oh no, this is a great point, so maybe we shouldn't get rid of these classes from our public API? 😭
There was a problem hiding this comment.
These classes should be part of the contract between muzzle generation and muzzle verification. But no end-user, including extension authors, should see them.
It bothered me for a long time that we have a bunch of public classes in
io.opentelemetry.javaagent.extension.muzzlepackage that in fact should never ever be used directly by the users ofjavaagent-extension-apimodule. They are there and public only becauseInstrumentationModule#getMuzzleReferences()exposes them. But that method is also should never be used by end-users. It is auto-generated and is used bymuzzleandtoolingmodules.This PR does the following:
InstrumentationModule#getMuzzleReferences()method from the source codeMuzzleReferencesAccessorclass intomuzzlemodule to access the generated version ofgetMuzzleReferencesmethod viaMethodHandlegetMuzzleReferencesmethod insideReferenceMatcherclass (andReferencesPrinter)All of the above would allow us to move
io.opentelemetry.javaagent.extension.muzzle.ClassRefclass (and all related classes from that package) fromjavaagent-extension-apimodule intomuzzle, but unfortunately they are used bygradle-plugins. So as preparation for the future, I created a verbatim copies of all classes fromio.opentelemetry.javaagent.extension.muzzlepackage inio.opentelemetry.javaagent.tooling.muzzle.references. Next PR can change gradle plugins to use these new classes. Old copies of them can be removed then fromjavaagent-extension-api.